All committers have signed the CLA.

cc @banks

Do you think this change would break anything in consul?

Hey, thanks for the PR!

It would be really useful if we could have the problem statement filled out in the PR description here (or link to an issue with the details) - IIRC Varint encoding does sort naturally on its own, can you give an example where they don't? Is it only when used as part of a composite index?

On the same note, adding a test case that fails with the existing implementation would be helpful.

I don't think this would break anything for Consul or other consumers given that the expectation is for integer fields to be sorted already, but would be great to understand how that is being violated! I don't imagine any callers should be reliant on the actual memory encoding of the index. I guess the one possible side effect I can see here is that in the worst case where only small numbers were indexed but the actual underlying type is int or int64 this could consume up to 7 bytes more RAM per key for the same workload. My guess is that's not a big enough problem if it's fixing broken behaviour though.

Hey, thanks for the PR!

It would be really useful if we could have the problem statement filled out in the PR description here (or link to an issue with the details) - IIRC Varint encoding does sort naturally on its own, can you give an example where they don't? Is it only when used as part of a composite index?

Varint may sort naturally (although that's suspect based on #77) but UVarInt definitely doesn't:
https://play.golang.org/p/an3xS-qlaEZ

On the same note, adding a test case that fails with the existing implementation would be helpful.

Added

I don't think this would break anything for Consul or other consumers given that the expectation is for integer fields to be sorted already, but would be great to understand how that is being violated! I don't imagine any callers should be reliant on the actual memory encoding of the index. I guess the one possible side effect I can see here is that in the worst case where only small numbers were indexed but the actual underlying type is int or int64 this could consume up to 7 bytes more RAM per key for the same workload. My guess is that's not a big enough problem if it's fixing broken behaviour though.

Because the index is allocating the same size buffer for each value based on the type of the value being indexed, this should strictly use less memory in all cases. uint8 takes two bytes when uvarint encoded, this takes 1. uint64 takes 10 bytes when varint encoded, this will use 8.

Tagged this in v1.3.2